Skip to content

bpo-38242: Revert "bpo-36889: Merge asyncio streams (GH-13251)"#16482

Merged
1st1 merged 1 commit intopython:masterfrom
1st1:revert
Sep 30, 2019
Merged

bpo-38242: Revert "bpo-36889: Merge asyncio streams (GH-13251)"#16482
1st1 merged 1 commit intopython:masterfrom
1st1:revert

Conversation

@1st1
Copy link
Member

@1st1 1st1 commented Sep 30, 2019 •
edited by bedevere-bot
Loading

See https://bugs.python.org/issue38242 for more details

99% of the work is done by @aeros167; I've just needed to add a few more fixes and couldn't push to his branch; hence the new PR.

https://bugs.python.org/issue38242

@1st1 1st1 requested a review from asvetlov as a code owner September 30, 2019 04:17
@1st1 1st1 mentioned this pull request Sep 30, 2019
Closed
aeros
aeros reviewed Sep 30, 2019
View reviewed changes
@aeros
Copy link
Contributor

aeros commented Sep 30, 2019

99% of the work is done by @aeros167; I've just needed to add a few more fixes and couldn't push to his branch; hence the new PR.

Thanks for finishing this up Yury. Out of curiosity, what additional changes needed to be made?

@1st1
Copy link
Member Author

1st1 commented Sep 30, 2019

Thanks for finishing this up Yury. Out of curiosity, what additional changes needed to be made?

.write() and .close() were still awaitable -- which technically was committed before new streams api. Still we want to revert streams to their 3.7 state.

aeros
aeros reviewed Sep 30, 2019
View reviewed changes
Doc/library/asyncio-stream.rst

.. coroutinefunction:: start_server(client_connected_cb, host=None, \
port=None, \*, loop=None, limit=2**16, \
port=None, \*, loop=None, limit=None, \
Copy link
Contributor

@aeros aeros Sep 30, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The constant _DEFAULT_LIMIT (2**16) is still being used as the default for limit in start_server(), so this can be restored.

Suggested change
port=None, \*, loop=None, limit=None, \
port=None, \*, loop=None, limit=2**16, \

Copy link
Contributor

@aeros aeros Sep 30, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can do this in a separate PR if needed. This change is rather low priority in comparison to reverting the streaming API, and would not be a release blocker.

Copy link
Member Author

@1st1 1st1 Sep 30, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, although right now docs are the same as in 3.7. So I'm not sure it's needed tbh.

Copy link
Contributor

@aeros aeros Sep 30, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I'm not sure it's needed tbh.

Okay, I won't worry about it then, it's fairly trivial.

@aeros
Copy link
Contributor

aeros commented Sep 30, 2019

.write() and .close() were still awaitable -- which technically was committed before new streams api. Still we want to revert streams to their 3.7 state.

Ah, I wasn't aware we were also removing that change. I hadn't realized we were reverting fully back to the 3.7 API, I thought it was primarily just focused on 23b4b69. Now it makes a lot more sense why you wanted to remove _asyncio_internal.

@aeros aeros mentioned this pull request Sep 30, 2019
Closed
@1st1 1st1 merged commit 6758e6e into python:master Sep 30, 2019
@1st1 1st1 deleted the revert branch September 30, 2019 04:59
@bedevere-bot
Copy link

bedevere-bot commented Sep 30, 2019

@1st1: Please replace # with GH- in the commit message next time. Thanks!

@miss-islington
Copy link
Contributor

miss-islington commented Sep 30, 2019

Thanks @1st1 for the PR 🌮🎉.. I'm working now to backport this PR to: 3.8.
🐍🍒⛏🤖

@miss-islington
Copy link
Contributor

miss-islington commented Sep 30, 2019

Sorry, @1st1, I could not cleanly backport this to 3.8 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 6758e6e12a71ef5530146161881f88df1fa43382 3.8

1st1 added a commit to 1st1/cpython that referenced this pull request Sep 30, 2019
@1st1
bpo-38242: Revert "bpo-36889: Merge asyncio streams (pythonGH-13251)" (…
01f4b6a 
…python#16482)

See https://bugs.python.org/issue38242 for more details
@bedevere-bot
Copy link

bedevere-bot commented Sep 30, 2019

GH-16485 is a backport of this pull request to the 3.8 branch.

1st1 added a commit that referenced this pull request Sep 30, 2019
@1st1
bpo-38242: Revert "bpo-36889: Merge asyncio streams (GH-13251)" (#16482…
1c19d65 
…) (#16485)

See https://bugs.python.org/issue38242 for more details
jacobneiltaylor pushed a commit to jacobneiltaylor/cpython that referenced this pull request Dec 5, 2019
@1st1
bpo-38242: Revert "bpo-36889: Merge asyncio streams (pythonGH-13251)" (…
62aedce 
…python#16482)

See https://bugs.python.org/issue38242 for more details
@arhadthedev arhadthedev mentioned this pull request Mar 22, 2022
Closed
@arhadthedev arhadthedev mentioned this pull request Apr 11, 2022
Merged
@1st1 1st1 mentioned this pull request Dec 28, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@asvetlov asvetlov Awaiting requested review from asvetlov

1 more reviewer

@aeros aeros aeros left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@1st1 1st1

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@1st1 @aeros @bedevere-bot @miss-islington @the-knights-who-say-ni